I think this is a good solution, but maybe we should also revert moving the files in the src directory, at least for now?

Otherwise it will only work if we release Gherkin and Behat at the same time (and bump the minimum version requirement in Behat).

Also Behat may not be the only thing using this class with a path to the Gherkin i18n file - I'm not sure the limited benefit we get from flattening the directory structure is worth even a small risk of causing problems for other people?

@acoulton I think that moving the files is a good move that we should keep. No one should have been depending on the location of files, and if they did they would need to work to fix that unnecessary dependency. But it is true that currently this makes the update cumbersome, having to release Gherkin and Behat at the same time. What I think we could do:

• Create here a new v4.12.0 release branch not from the current master but from a previous point in the code before the files were moved.
• Move this PR to point to this new branch (adjusting the file location to the old location)
• Once this PR is merged, release the v4.12.0 version. This would provide a version that anyone could use to break the link with the location of the file. It would work both with code which provided the file location and for code that did not
• Merge this release branch into master, adjusting the file location to the new location
• Update Behat to require version v4.12.0 and adjust the code to stop sending the file location parameter. We would then have a Behat version that we could release whenever convenient

What do you think?

No one should have been depending on the location of files, and if they did they would need to work to fix that unnecessary dependency.

Normally I would agree (and did when I merged #288).

However, discovering now that we ship a class that it seems would always/usually need to be used with a path in this repo it seems a little more reasonable that people might have being doing that...

But I think your solution is good, I think the majority of users will be using this with Behat so as long as that upgrade path is smooth that's probably ok.

@acoulton I created the new release branch, modified this PR to point to the new branch and modified the code to point to the old location

@carlos-granados thanks - I noticed there's a test that was also specifying the path (both in the setup and the assertions). I've pushed a commit to your branch that removes the constructor argument, so that should test should now prove that the default file location is correct (it will fail when the files are moved).

While doing that, I wondered if it would be clearer to use a named constructor rather than an optional argument - something like:

public static function withGherkinI18n(): self {
    // or maybe withDefaultKeywords // withDefaultTranslations?
    return new self(__DIR__ . '/../../../../i18n.php');
}

// and then leave the constructor parameter required.

That way it's explicit to end users what this class does with a null / missing constructor argument - CachedArrayKeywords alone isn't that obvious what it's actually loading / caching...

What do you think?

@carlos-granados thanks - I noticed there's a test that was also specifying the path (both in the setup and the assertions). I've pushed a commit to your branch that removes the constructor argument, so that should test should now prove that the default file location is correct (it will fail when the files are moved).

While doing that, I wondered if it would be clearer to use a named constructor rather than an optional argument - something like:

public static function withGherkinI18n(): self {
    // or maybe withDefaultKeywords // withDefaultTranslations?
    return new self(__DIR__ . '/../../../../i18n.php');
}

// and then leave the constructor parameter required.

That way it's explicit to end users what this class does with a null / missing constructor argument - CachedArrayKeywords alone isn't that obvious what it's actually loading / caching...

What do you think?

Good idea, I think that withDefaultKeywords would be the best option

Good idea, I think that withDefaultKeywords would be the best option

I can update that in this PR, unless you want to?

Good idea, I think that withDefaultKeywords would be the best option

I can update that in this PR, unless you want to?

Yes, please, go ahead I am busy with something else at the moment

@carlos-granados done - let me know if this looks OK to you and I can squash-merge and do the release notes.